dgram: add custom lookup function in sockets #14560
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
the
dgram
|
semver-minor? |
cjihrig
added
the
semver-minor
|
Yes, added the label. Thanks. |
jasnell
approved these changes
Aug 1, 2017
geek
reviewed
Aug 1, 2017
| * `reuseAddr` {boolean} When `true` [`socket.bind()`][] will reuse the | ||
| address, even if another process has already bound a socket on it. Optional. | ||
| Defaults to `false`. | ||
| * `lookup` {Function} Custom lookup function. Defaults to [`dns.lookup()`][]. |
There was a problem hiding this comment.
Would be nice to provide the function signature
There was a problem hiding this comment.
I copied that directly from the net docs for the same feature.
geek
approved these changes
Aug 1, 2017
evanlucas
approved these changes
Aug 2, 2017
This commit adds support for custom DNS lookup functions in dgram sockets. This is similar to the existing feature in net sockets. Refs: nodejs#6189 PR-URL: nodejs#14560 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Wyatt Preul <wpreul@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
|
This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it just add the |
maclover7
referenced
this pull request
in maclover7/node
Sep 17, 2017
Will break YAML parsing!
Original Node error:
```
Jonathans-MBP:node jon$ node tools/doc/generate.js --format=json doc/api/dgram.md
Input file = doc/api/dgram.md
{ Error
at generateError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:165:10)
at throwError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:171:9)
at readBlockSequence (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:935:7)
at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1331:12)
at readBlockMapping (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1062:11)
at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1332:12)
at readDocument (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1492:3)
at loadDocuments (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1548:5)
at load (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1569:19)
at Object.safeLoad (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1591:10)
name: 'YAMLException',
reason: 'bad indentation of a sequence entry',
mark:
Mark {
name: null,
buffer: '\nadded: v0.11.13\nchanges:\n - version: REPLACEME\n pr-url: https://github.com/nodejs/node/pull/14560\n description: The `lookup` option is supported.\n - version: REPLACEME\n pr-url: https://github.com/nodejs/node/pull/13623\n description: `recvBufferSize` and `sendBufferSize` options are supported now.\n\u0000',
position: 248,
line: 8,
column: 17 },
message: 'bad indentation of a sequence entry at line 9, column 18:\n description: `recvBufferSize` and `sendBuffer ... \n ^' }
```
Then I extracted out the problematic YAML, and tried running through Ruby as a
separate `.yml` file:
```
Psych::SyntaxError: (<unknown>): found character that cannot start any token while scanning for the next token at line 8 column 18
from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse'
from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse_stream'
from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:325:in `parse'
from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:252:in `load'
from (irb):7
from /Users/jon/.rbenv/versions/2.4.0/bin/irb:11:in `<main>'
```
|
Ping @cjihrig ... Initial basic attempts to backport this for v8.x are failing due to some issue with dns. Will investigate further shortly. |
|
@jasnell not sure. The commit in this PR lands cleanly for me on v8.x-staging. I ran the test suite locally and only saw one unrelated failure (test/sequential/test-benchmark-http.js). |
|
It's likely that the commits I landed today unblocked it. I'll try again tomorrow |
jasnell
added a commit
that referenced
this pull request
Sep 26, 2017
* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
jasnell
added a commit
that referenced
this pull request
Sep 26, 2017
* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
MylesBorins
pushed a commit
that referenced
this pull request
Sep 29, 2017
* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
addaleax
pushed a commit
to addaleax/ayo
that referenced
this pull request
Sep 30, 2017
* **crypto** * Support for multiple ECDH curves. [#15206](nodejs/node#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](nodejs/node#7855) * Custom lookup functions are now supported. [#14560](nodejs/node#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](nodejs/node#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](nodejs/node#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](nodejs/node#15354)
|
Release team were -1 on landing on v6.x, if you disagree let us know. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds support for custom DNS lookup functions in dgram sockets. This is similar to the existing feature in net sockets.
Refs: #6189
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
dgram